`Result` ALL THE THINGS!!! by designatednerd · Pull Request #644 · apollographql/apollo-ios

designatednerd · 2019-07-17T14:23:24Z

This PR changes pretty much all of our completion closures to take advantage of Swift 5's built-in Result type. This is going to be a SUPER-BREAKING CHANGE™, though I believe it will help significantly in disambiguating whether a particular request has succeeded or failed, which is why I've done it in the first place.

If you have reasons you think I should abandon this path, I'd love to hear them - please let me know in the comments!

In this PR:

Changed basically anything which had the following signature:
```
completionHandler: ((Object?, Error?) -> Void)
```
to use this signature instead:
```
completionHandler: ((Result<Object, Error>) -> Void)
```
This takes advantage of Swift's built in Result type to make it WAY easier to tell when a request has failed prior to parsing. This does cause a couple of significant changes:
- Previously, there were cases where we would return neither a result nor an error. These cases will now always return a result.
- This leads to some extra-fun and inscrutable compiler errors when you're going through and updating everything:
  
  What this error is attemping to tell you is that you need to change the two-parameter completion closure into a single-parameter one. It's telling you that in a very, very opaque fashion, though. I plan to document this in the release notes as well.
Updated all tests - I'll flag the ones which are significant changes in comments.
Removed deprecated method OperationResultHandler typealias since I'm already breaking the crap out of everything

designatednerd · 2019-07-17T14:40:50Z

+            } else {
+              XCTFail("Unexpected error: \(error)")
+            }
+          }


This test represents a breaking change: Before if you cleared the cache and requested the same info again, you would get nil for both the GraphQLResult and the Error. Now you will receive an error since there is nothing to decode for that result.

That definitely seems correct to me, I think the previous behavior was a mistake.

designatednerd · 2019-07-17T14:42:51Z

+            let websocketError = WebSocketError(payload: payload,
+                                                error: error,
+                                                kind: .neitherErrorNorPayloadReceived)
+            responseHandler(.failure(websocketError))


This is a new error type to help handle cases where we previously could have sent nil for both the payload and the error.

I don't have the full context here (this was contributed by someone else), but I'm wondering when this condition would actually occur. I suspect this is something that should be solved in OperationMessage.parse, because it seems like a protocol error.

Yeah I plan to look at that in more depth when I do some much more detailed looks at the WebSocket stuff- this is at least to get this out the door in a way that still gives us some kind of semi-meaningful error about what went wrong.

designatednerd · 2019-07-17T14:44:30Z

+          } else {
+            XCTFail("Unexpected error: \(error)")
+          }
+        }


This test also represents a breaking change - I think that this has an outer GraphQLResultError since there's some data in the cache, but not all of it.

designatednerd · 2019-07-17T20:01:07Z

Twitter feedback on this change has been positive so far - gonna let @martijnwalraven tell me what I'm doing wrong tomorrow and then leave it open for further feedback for a day or two.

martijnwalraven · 2019-07-19T07:30:51Z

-
-@available(*, deprecated, renamed: "GraphQLResultHandler")
-public typealias OperationResultHandler<Operation: GraphQLOperation> = GraphQLResultHandler<Operation.Data>
+///   - result: The result of a performed operation. Will have a `GraphQLResult` with the data of the requested type on `success`, and an `Error` on `failure`.


Not sure if this is needed, but we may want to mention GraphQLResult can contain GraphQL errors in addition to data.

Yeah good call

martijnwalraven · 2019-07-19T07:35:44Z


  func fetch(cachePolicy: CachePolicy) {
-    fetching = client?.fetch(query: query, cachePolicy: cachePolicy, context: &context) { [weak self] (result, error) in
+    fetching = client?.fetch(query: query, cachePolicy: cachePolicy, context: &context) { [weak self] outerResult in


I'm wondering if we can find a more descriptive name than outerResult, but I don't have any great suggestions. Or maybe just result and graphQLResult?

yeah that works - i'll try to change that throughout when there's not a 2nd result being wrapped

martijnwalraven · 2019-07-19T07:41:39Z

+            let websocketError = WebSocketError(payload: payload,
+                                                error: error,
+                                                kind: .neitherErrorNorPayloadReceived)
+            responseHandler(.failure(websocketError))


I don't have the full context here (this was contributed by someone else), but I'm wondering when this condition would actually occur. I suspect this is something that should be solved in OperationMessage.parse, because it seems like a protocol error.

martijnwalraven · 2019-07-19T07:44:37Z

+            } else {
+              XCTFail("Unexpected error: \(error)")
+            }
+          }


That definitely seems correct to me, I think the previous behavior was a mistake.

martijnwalraven · 2019-07-19T07:45:57Z

+        }

-        client.fetch(query: query, cachePolicy: .returnCacheDataDontFetch) { (result, error) in
+        client.fetch(query: query, cachePolicy: .returnCacheDataDontFetch) { innerResult in


Why is this called innerResult here?

Because this is a nested callback - we already have outerResult above.

mormahr · 2019-07-19T17:00:23Z

Hey, not an expert wrt. swift, but maybe one could add the old signature with a @deprecated or something like that?

designatednerd · 2019-07-19T17:32:43Z

@mormahr Definitely doable, but it's pretty significant maintenance overhead to maintain all of the old public method signatures and basically need to do this with every single one:

@available(*, deprecated, message: "Use the method with the `Result` completion handler instead")
func oldMethod(completion: @escaping (_ parameter: Parameter?, _ error: Error?)) {
  self.newMethod { result in 
    switch result {
    case .success(let parameter):
      completion(parameter, nil)
    case .failure(let error):
      completion(nil, error)
    }
  }
}

Unless there's a real good reason not to, I'd kinda prefer to rip the band-aid off here.

designatednerd · 2019-07-20T11:57:49Z

OK folks - leaving this open for further comment until 10pm Netherlands Time (1pm US-Pacific) on Monday. Speak now or forever be mad at me for breaking your shit!

…nforming to the `NetworkTransport` protocol

…date docs

…gate` into extensions.

Yo dawg I heard you like Result so I put a Result in your Result

designatednerd · 2019-07-22T20:27:20Z

May [deity of choice] have mercy on us all. Merging!

JakeTorres · 2019-07-30T14:18:18Z

what's wrong with that, get a schema.json, but swift API

designatednerd commented Jul 17, 2019

View reviewed changes

designatednerd requested a review from martijnwalraven July 17, 2019 14:59

designatednerd added this to the 0.13.0 milestone Jul 17, 2019

designatednerd added enhancement Issues outlining new things we want to do or things that will make our lives as devs easier swift-5 Issues specific to Swift 5.0 labels Jul 17, 2019

designatednerd force-pushed the change/result-type-returns branch from 0e9fc98 to bda9b6a Compare July 17, 2019 20:09

martijnwalraven approved these changes Jul 19, 2019

View reviewed changes

designatednerd force-pushed the change/result-type-returns branch from bda9b6a to d644a75 Compare July 19, 2019 16:28

designatednerd added 16 commits July 22, 2019 15:22

remove deprecated handler type

7b9c291

Switch to Result type with the GraphQLResultHandler

52e1d8a

update ApolloTests and ApolloCacheDependentTests

586a55e

update SQLite tests for result type

339b1f9

Fix placement of call to go out to network

f772a2a

Check for correct errors in tests

f485989

Update Websocket to use swift result for completion handlers

801efb9

fix typo in test class name

0bc5833

Update web socket tests for result type

b42cc37

Update documentation

6ca1e8a

move send into an extension to make it clearer that this method is co…

4be1186

…nforming to the `NetworkTransport` protocol

rm whitespace

94a0073

pull out split network transport's NetworkTransport conformance; up…

0c059b4

…date docs

pull out WebSocketTransport's NetworkTransport and `WebSocketDele…

647a7da

…gate` into extensions.

rm excess whitespace 🤦‍♀️

3725fe0

Clarify header comment a bit

acf11ee

designatednerd added 5 commits July 22, 2019 15:24

rm excess whitespace

f9bafbf

simplify check

6eac173

clarify that GraphQLResult has both data and errors

b29b681

Yo dawg I heard you like Result so I put a Result in your Result

outerResult -> result anywhere there isn't actually an inner result.

8c4cbde

rm whitespace

c8b94d8

designatednerd force-pushed the change/result-type-returns branch from daca9bd to c8b94d8 Compare July 22, 2019 13:25

designatednerd merged commit 96305f8 into master Jul 22, 2019

designatednerd deleted the change/result-type-returns branch July 23, 2019 09:28

casey-chow mentioned this pull request Jul 23, 2019

Can't access result from mutation #663

Closed

giladronat mentioned this pull request Jul 25, 2019

Update Usage docs to use Result #669

Merged

Conversation

designatednerd commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

designatednerd commented Jul 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mormahr commented Jul 19, 2019

Uh oh!

designatednerd commented Jul 19, 2019

Uh oh!

designatednerd commented Jul 20, 2019

Uh oh!

designatednerd commented Jul 22, 2019

Uh oh!

JakeTorres commented Jul 30, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

designatednerd commented Jul 17, 2019 •

edited

Loading